Skip to content

s3 pagination failures #1177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 30, 2025
Merged

s3 pagination failures #1177

merged 2 commits into from
May 30, 2025

Conversation

smallhive
Copy link
Contributor

Closes #1175

The tests themselves need to be updated as well

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive requested a review from roman-khimov as a code owner May 26, 2025 10:59
@smallhive
Copy link
Contributor Author

Change 1

This subpart of the test_s3_pagination_in_objects_versions_listing_via_boto3 fails each time, even without S3 gate communication

...
        with allure.step("Check pagination with starting token"):
            paginator = self.s3_client.get_paginator("list_object_versions")
    
            next_token = None
            all_versions = []
    
            while True:
                pagination_args = {"Bucket": bucket, "PaginationConfig": {"PageSize": 5}}
                if next_token:
                    pagination_args["PaginationConfig"]["StartingToken"] = next_token
    
                page_iterator = paginator.paginate(**pagination_args)
>               page = next(page_iterator)
E               TypeError: 'PageIterator' object is not an iterator

with error

FAILED pytest_tests/tests/s3/test_s3_versioning.py::TestS3Versioning::test_s3_pagination_in_objects_versions_listing_via_boto3[boto3] - TypeError: 'PageIterator' object is not an iterator

@smallhive
Copy link
Contributor Author

Change 2

I consider that there is a logical error in the test_s3_pagination_in_objects_versions_listing_via_boto3 test.

We took max_keys (5 in the original code) and save it into versions. In the next subtest, we check these 5 items with a fully collected bucket, as a result 20 != 5.

We should update the test in the required way. Either change max_keys or get versions one more time on the next test

def test_s3_pagination_in_objects_versions_listing_via_boto3(self, prepare_versioned_objects: tuple):
        bucket, objects_count = prepare_versioned_objects
        with allure.step("Check basic pagination with MaxKeys"):
            max_keys = 5

to

def test_s3_pagination_in_objects_versions_listing_via_boto3(self, prepare_versioned_objects: tuple):
        bucket, objects_count = prepare_versioned_objects
        with allure.step("Check basic pagination with MaxKeys"):
            max_keys = objects_count

or another way, if the original assumption was different

@smallhive
Copy link
Contributor Author

smallhive commented May 26, 2025

Change 3

inside test_s3_pagination_in_objects_versions_listing_via_cli

assert len(versions) == max_keys, f"Expected {max_keys} versions, got {len(versions)}"

replace with

assert len(versions) <= max_keys, f"Expected {max_keys} versions, got {len(versions)}"

The reason is here.
There are NeoFS search v2 cursor implementation-specific things against ListObjectVersions required behaviour

@smallhive smallhive requested a review from evgeniiz321 May 26, 2025 11:02
@smallhive smallhive force-pushed the 1175-s3-pagination-failures branch from 57674ff to d2aa83d Compare May 26, 2025 11:44
@evgeniiz321
Copy link

evgeniiz321 commented May 26, 2025

@smallhive
Change 1 - fixed
Change 2 - there was just a comparison with a wrong variable there - fixed it as well
Change 3 - fixed
But tests still fail on the original problem - The same next token was received twice from this PR as well

@roman-khimov
Copy link
Member

@smallhive: all of the explanations belong to the commit message. That's exactly what it is for.

@smallhive smallhive force-pushed the 1175-s3-pagination-failures branch from d2aa83d to 0acea6f Compare May 27, 2025 10:21
@smallhive smallhive force-pushed the 1175-s3-pagination-failures branch from 0acea6f to 5d8bd0c Compare May 29, 2025 06:55
@smallhive smallhive force-pushed the 1175-s3-pagination-failures branch from 5d8bd0c to 8c64adc Compare May 30, 2025 09:08
@smallhive
Copy link
Contributor Author

Reworked

@smallhive smallhive requested a review from roman-khimov May 30, 2025 09:13
@smallhive smallhive force-pushed the 1175-s3-pagination-failures branch from 8c64adc to c2be36d Compare May 30, 2025 09:57
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably OK, we need more tests.

Closes #1175.

ListObjectVersions method returns objects in the order that they were stored,
returning the most recently stored object first.
See details https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectVersions.html.

To achieve this behaviour we extend `nextVersionIDMarker` with timestamp. It is used to
filter out already processed objects.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 1175-s3-pagination-failures branch from c2be36d to eaf71c9 Compare May 30, 2025 10:10
@smallhive smallhive requested a review from roman-khimov May 30, 2025 10:11
@roman-khimov roman-khimov merged commit eab519e into master May 30, 2025
12 of 15 checks passed
@roman-khimov roman-khimov deleted the 1175-s3-pagination-failures branch May 30, 2025 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

s3 pagination failures
3 participants